-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: headers refactoring and state transition invariants #80
Conversation
58d12c2
to
f867a7e
Compare
@@ -111,7 +111,7 @@ class Headers final : public BuiltinImpl<Headers> { | |||
static JSObject *create(JSContext *cx, host_api::HttpHeadersReadOnly *handle, | |||
host_api::HttpHeadersGuard guard); | |||
|
|||
static JSObject *init_entries(JSContext *cx, HandleObject self, HandleValue init_headers); | |||
static bool init_entries(JSContext *cx, HandleObject self, HandleValue init_headers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to make a decision at some point about how we version starlingmonkey and how we communicate public function signatures changes such as this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From js-compute-runtime's perspective, every StarlingMonkey commit is potentially a breaking change. There simply are no API guarantees.
But since these are changes on the js-compute-runtime -> StarlingMonkey interface, they are all manageable in theory.
Problems come into play if:
- A StarlingMonkey change causes the corresponding js-compute-runtime upgrade to be an inhibitative amount of work.
- A StarlingMonkey change causes a breaking change in js-compute-runtimes's public guest API usage for JS users.
I hope our implicit agreement is effectively that we could revert a change in the case of (1) or (2), but perhaps we should discuss this more with @tschneidereit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this as a sentiment for now, yes. Eventually I would like to get to a more stable API, but I don't think the one we have right now is sufficiently well designed to declare it stable as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good insight, thank you!
I think there's a slight change still to be made—see the comments in-line, but then it'll be good to go.
Co-authored-by: Till Schneidereit <[email protected]>
This includes some simple refactoring including unused code paths and also adds some extra state assertions, including encoding the state transition invariant that we never transition from
Uninitialized
toHostOnly
. If this invariant changes in future we can always relax it again, but it helped my understanding of the state machine to note these assertions.